Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/complete module #2340

Merged
merged 2 commits into from
Jun 11, 2021
Merged

Conversation

DanielaE
Copy link
Contributor

@DanielaE DanielaE commented Jun 4, 2021

Add all missing pieces to complete {fmt} as module:

  • support non-char overloads
  • support compile-time argument type checking
  • support compile-time format string compilation

This includes the macro-based API FMT_STRING and FMT_COMPILE by means of a module companion header fmt.h

using detail::specs_handler;
using detail::to_unsigned;
using detail::type;
using detail::write;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure, if this is really needed by the standard or if this is just an implementation deficit. At least it looks ugly.

From reading "C++ Templates - The Definitive Guide" and the standard, it is clear that the local class is a templated entity. I need further studies of the standard to find an answer to the question: which names within this so-called 'temploid' are bound at which time.

I'd rather prefer to make this 'temploid' a proper class template.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which local class are you referring to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

struct format_handler within vformat_to in format.h, currently line 2649.

The problem is explored with this stripped-down code example here https://godbolt.org/z/34Ph9dee9. Fortunately I got this compiled with gcc (modules-branch) for the first time just a few minutes ago, so that I have a second opinion on the matter now: gcc doesn't require this 'symbolic-linking' of names into the body of vformat_to and all of my studies of the standard seem to corroborate that. Therefore I consider this an implementation bug in latest msvc. This symbolic linking is the only workaround that works at all. Qualified name lookup doesn't. And only local classes within function templates are affected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is acknowledged as bug in msvc by realiable sources familiar with the compiler: name-binding is happening in phase 2 instead of phase 1 in this particular case. In other words: too late to find anything in namespace detail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a short comment clarifying that this is a workaround for an msvc bug?

@DanielaE DanielaE marked this pull request as ready for review June 5, 2021 11:17
Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

In general looks good but i don't think we should be introducing a new header for macros because FMT_STRING and FMT_COMPILE are effectively obsolete with C++20. The former is redundant and the latter is replaced with a UDL.

FMT_MODULE_EXPORT_BEGIN

// compile-time support
namespace ct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why new namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is stuff that was in namespace detail before. It must be made visible to be used from importers, f.e. by the ""_cf udl. The compiled_string tag class can be moved into namespace fmt just as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we make detail::compiled_string visible? It's an implementation detail and not supposed to be directly used by anyone, so detail is the correct namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make detail::compiled_string visible then the namespace detail becomes implicitly visible, too. This disenfranchises testing from ever checking if anything from that namespace becomes unwittingly visible that was never meant to be visible int the first place. This is the sole reason why I introduced namespace ct: everything in there is the sole minimum of non-public API that needs to be visible. I'd be sad if we'd call namespace detail "open season" to visibility. I was bugging Cameron to fix the current issue in msvc of detail being visible even if nothing in there is currently visible (he checked this with his BMI explorer tool) and he told me that this should be easy to fix. 🤞

I've just finished reworking the PR and peeked here to check for any news. Part of the rework was moving compiled_string to namespace fmt because I got the feeling you were objecting to putting it into ct where other necessarily visible compile-time related stuff is moved to as well. I will push the PR as it is now. Changing it in either direction is a piece of cake and can be done within a few minutes.

Copy link
Contributor

@vitaut vitaut Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not making detail visible makes sense but ct looks like part of the public API which is undesirable. I suggest renaming ct to something like detail_exported to make it clear that this is an internal API but exported for module purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines +131 to +141
std::wstring w;
fmt::format_to(std::back_inserter(w), L"{}", 42);
EXPECT_EQ(L"42", w);

wchar_t wbuffer[4] = {0};
fmt::format_to(wbuffer, L"{}", 42);
EXPECT_EQ(L"42", std::wstring_view(wbuffer));

fmt::wmemory_buffer wb;
fmt::format_to(wb, L"{}", 42);
EXPECT_EQ(L"42", std::wstring_view(wbuffer));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As before, I think this does too much functional testing. I suggest having a single wide format_to call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are different overloads in the API. I think this is the minimal set for full coverage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's keep them.

Comment on lines 254 to 255
EXPECT_WRITE(stdout, fmt::print(L"{}µ", 42), as_string(L"42µ"));
EXPECT_WRITE(stderr, fmt::print(stderr, L"{}µ", 4.2), as_string(L"4.2µ"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be problematic because IIRC they may fail if the stream is not in wide mode. I suggest only checking that this compiles without actually running, i.e. wrap in if (false).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will do.

@DanielaE
Copy link
Contributor Author

DanielaE commented Jun 9, 2021

Even better then. I'm not keen on seeing macros in any API.

FMT_STRING being obsolete is news to me. AFAICS the code paths are different with and without FMT_STRING. And it is used internally too which was giving me quite some head-scratching how to make these internal uses work, not to mention external uses. Cameron was kindly helping me analyzing the problem.

Both macros are listed in the documentation as official part of the API and this is why I felt like providing them.

@vitaut
Copy link
Contributor

vitaut commented Jun 10, 2021

FMT_STRING will still be supported for pre-C++20 compilers but since modules are C++20 we don't need to export it here.

@DanielaE
Copy link
Contributor Author

BTW, I've removed the block in my local repo that prevents msvc from running the enforce-checks-test: it passes the test with flying colours. The full CI matrix for Windows is green.

FMT_MODULE_EXPORT_BEGIN

// A compile-time string which is compiled into fast formatting code.
class compiled_string {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it still need to be public if we don't export the FMT_COMPILE macro and only provide a UDL?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted - thanks!
All related stuff removed.

@DanielaE DanielaE force-pushed the feature/complete-module branch 2 times, most recently from 669e148 to 60cdb0e Compare June 11, 2021 13:32
@@ -164,7 +164,7 @@ struct is_compiled_string : std::is_base_of<compiled_string, S> {};
#endif

#if FMT_USE_NONTYPE_TEMPLATE_PARAMETERS
template <typename Char, size_t N, fixed_string<Char, N> Str>
template <typename Char, size_t N, fmt::detail_exported::str<Char, N> Str>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest keeping the old name (fixed_string) which is more descriptive.

Copy link
Contributor Author

@DanielaE DanielaE Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.
My concern is the size of these fmt::v7::detail_exported::fixed_string linker symbols. This is just the common prefix. If you look at real code, the actual linker symbols with the specialized fixed strings become horribly long. May be I can convince Cameron to use a different mangling, may be derived from the base64-encoded SHA1 of the string value.

Anyway, I will change that.

Comment on lines 793 to 805
// Converts a compile-time string to basic_string_view.
template <typename Char, size_t N>
constexpr auto string_to_view(const Char (&s)[N]) -> basic_string_view<Char> {
// Remove trailing NUL character if needed. Won't be present if this is used
// with a raw character array (i.e. not defined as a string).
return {s, N - (std::char_traits<Char>::to_int_type(s[N - 1]) == 0 ? 1 : 0)};
}
template <typename Char>
constexpr auto string_to_view(detail::std_string_view<Char> s)
-> basic_string_view<Char> {
return {s.data(), s.size()};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are only used in macros that are not exported so probably should be in the detail namespace, not detail_exported. Also please keep the old names (compile_string_to_view).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. These overloads must be visible or xchar.h:231 will fail to compile. And there is no workaround for that.
I will change the name. Even though nothing is compiled here but just a view to the stored data created.

…on in module

Make just the necessary parts available for lookup from client context.
@vitaut vitaut merged commit 55010a9 into fmtlib:master Jun 11, 2021
@vitaut
Copy link
Contributor

vitaut commented Jun 11, 2021

Thank you!

@DanielaE DanielaE deleted the feature/complete-module branch June 11, 2021 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants